Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.5] Fixes application console method run #20922

Merged
merged 3 commits into from
Sep 2, 2017
Merged

[5.5] Fixes application console method run #20922

merged 3 commits into from
Sep 2, 2017

Conversation

nunomaduro
Copy link
Member

@nunomaduro nunomaduro commented Sep 1, 2017

This PR address an issue introduced on Laravel 5.5.

In the usage of the public method run, before we could actually use null as input and output. Since the Symfony's method run creates a default input and output.

Since now we are managing our run method we need to also create default values, in order to don't touch on the API of this method.

@themsaid
Copy link
Member

themsaid commented Sep 1, 2017

What issue does this PR fix? nothing seems to be broken and the method calls parent::run which creates the input/output instances as before

@nunomaduro
Copy link
Member Author

nunomaduro commented Sep 1, 2017

@themsaid It fixes illuminate/console component usage of the method run without default input/output. On Laravel that don't happens, because we pass all the time an valid input/output. Other packages may trust on the definition of the api of the method and pass null.

Right now, if we pass null on both params, the code will not run and will break on the following places:

Line 76:

$commandName = $this->getCommandName($input);

Error: ... must be an instance of Symfony\Component\Console\Input\InputInterface, null given.

Line 78:

        $this->events->fire(
            new Events\CommandStarting($commandName, $input, $output)
        ); 

Error: PHP Fatal error: Uncaught TypeError: Argument 3 passed to Illuminate\Console\Events\CommandStarting::__construct() must implement interface Symfony\Component\Console\Output\OutputInterface, null given.

@themsaid
Copy link
Member

themsaid commented Sep 1, 2017

I see, thanks for explaining :)

@paulofreitas
Copy link
Contributor

paulofreitas commented Sep 2, 2017

Commit b26a9fa overloaded the run() method to introduce the new console events. Symfony will still handle this itself, but the new console events require an InputInterface instance themselves.

Since Laravel 5.5 was now released changing the new console events signature to be nullable would certainly be a breaking change (it depends on BC promises), so for now it's safest to handle this early and postpone the interface signature change to the next major release. 👍

@taylorotwell taylorotwell merged commit 59083e9 into laravel:5.5 Sep 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants